gh-133009: fix UAF in xml.etree.ElementTree.Element.__deepcopy__
#133010
Conversation
There was a problem hiding this comment.
This is not enough. "tag", "text" and "tail" can be set to different value during deepcopying. If it was the last reference, the deepcopy() argument can be destroyed, and the reference can became handling.
The safe way is to increase the reference count of the deepcopy() argument before calling any code that can release the GIL.
|
I actually tried to do something with tag etc, but I wasn't able to crash the interpreter. |
|
String literals are interned and saved in the constants list. You need to use something having a single reference. Try |
I've actually removed this code because I thought it wasn't needed, but I'll check with an evil non-interned string tomorrow. |
|
I'll try to add more tests later as well. |
Modules/_elementtree.c
Outdated
|
|
||
| if (Py_REFCNT(object) == 1) { | ||
| if (PyDict_CheckExact(object)) { | ||
| // Exact dictionaries do not execute arbitrary code as it's |
There was a problem hiding this comment.
@serhiy-storchaka is this assumption correct? namely, here I don't need to incref object temporarily right?
There was a problem hiding this comment.
PyDict_Next() does not use __iter__, so this comment is redundant. PyDict_Next() does not call any user code.
Modules/_elementtree.c
Outdated
|
|
||
| if (Py_REFCNT(object) == 1) { | ||
| if (PyDict_CheckExact(object)) { | ||
| // Exact dictionaries do not execute arbitrary code as it's |
There was a problem hiding this comment.
PyDict_Next() does not use __iter__, so this comment is redundant. PyDict_Next() does not call any user code.
|
Actually, it's much more tricky than what I thought. Even with the INCREF/DECREF and the additional checks, the following still crashes, but not during class Evil(ET.Element):
def __deepcopy__(self, memo):
root.append(ET.Element('y'))
root.append(ET.Element('z'))
return self
Y = Evil('y')
root = ET.Element('a')
root.extend([Evil('x'), ET.Element('t'), Y])
c = deepcopy(root)
print(list(c))
print("ok")
assert 0So I'll need a bit more work. |
|
#133010 (comment) should help. It is good that you already have a reproducer. |
|
Ah, you have a different issue -- growing children, not attributes. The solution should be the same -- either ignore new items or resize the array in process. |
Yes, actually I found a way to fix it in the meantime (but the same can be said) |
| root = ET.Element('a') | ||
| evil = X('x') | ||
| root.extend([evil, ET.Element('y')]) | ||
| if is_python_implementation(): |
There was a problem hiding this comment.
We could also make the C implementation raising RuntimeError. It is fine either way.
There was a problem hiding this comment.
I'll postpone this for a future PR as I want to backport this one to 3.13 and 3.14 first.
|
Thanks @picnixz for the PR 🌮🎉.. I'm working now to backport this PR to: 3.13, 3.14. |
…__` (pythonGH-133010) (cherry picked from commit 116a9f9) Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
…__` (pythonGH-133010) (cherry picked from commit 116a9f9) Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
|
GH-133805 is a backport of this pull request to the 3.14 branch. |
|
GH-133806 is a backport of this pull request to the 3.13 branch. |
3 participants
xml.etree.ElementTree.Element.__deepcopy__when concurrent mutations happen #133009